-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHP 8.1: Added support for enum
keyword
#3478
Conversation
7f1bd6b
to
8be91bc
Compare
enum
keyword
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kukulich 👋🏻 Thank you for creating this PR. Looking good!
I've left some comments inline to think over. Hope it helps.
a86c6de
to
d709d56
Compare
Thanks for the update @kukulich, left one more comment. Other that that, 👍🏻 |
d709d56
to
94e80e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't verified against the RFC, I trust @kukulich will have done that, but PR-wise, this looks to cover all the bases 💯
42551e5
to
eb27ac3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, still found some more issues to be addressed.
tests/Core/Tokenizer/EnumTest.php
Outdated
* | ||
* @return void | ||
*/ | ||
public function testEnums($testMarker, $expectedScopeOpenerLine, $expectedScopeCloserLine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering - why test the line and not the actual token position (in relation to the keyword token) ?
I realize the latter is more fiddly for setting up the tests, but it's also much more precise.
In other tests, this has been done by the data provider providing an offset
and the token position then being calculated by $expectedOpener = $enum + $openerOffset;
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you modified it for scope opener, but removed the check for scope closer ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added now.
3e94231
to
bca788c
Compare
a7a0128
to
106eba3
Compare
What is the current state of this one? |
106eba3
to
f236c6a
Compare
Rebased on master. |
$tokens = self::$phpcsFile->getTokens(); | ||
|
||
$target = $this->getTargetToken($testMarker, [T_ENUM, T_STRING], $testContent); | ||
$this->assertSame(T_STRING, $tokens[$target]['code']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the way namespaces are tokenized in 4.0 (they become T_NAME_RELATIVE
or T_NAME_QUALIFIED
) I had to reverse this logic in the 4.0 branch to use assertNotSame(T_ENUM...)
. I didn't make the same change in master.
Thanks a lot for this PR. I really appreciate the effort you're putting in to add support for 8.1 features to PHPCS. |
Fixes #3474